fast-get: Fix shared data usage by multiple callers #10484
Conversation
fast_get() for the same data can be called by multiple clients from the same or other cores. Since sram_ptr points to cached memory, writeback is needed after memcpy() to also copy data to uncached memory. When fast_get() is called a second time for the same data, invalidate() is used to bring shared data into another core's cache. This fixes tests which use two SRC modules. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
serhiy-katsyuba-intel
requested review from
abonislawski,
dbaluta,
iuliana-prodan,
kv2019i and
lyakh
as code owners
|
@lgirdwood, Zephyr / build-linux (zmain, imx8 imx8x imx8m imx8ulp) CI failes with |
There was a problem hiding this comment.
Pull request overview
This PR fixes a cache coherency issue in the fast_get() function when the same data is accessed by multiple callers across different cores. The changes ensure proper cache alignment and synchronization for shared memory access.
Changes:
- Modified memory allocation to use cache-line alignment instead of no alignment
- Added cache writeback operation after copying data to ensure visibility across cores
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Comments inline but looking at the modified code alone, seems good.
| entry->size = size; | ||
| entry->sram_ptr = ret; | ||
| memcpy_s(entry->sram_ptr, entry->size, dram_ptr, size); | ||
| dcache_writeback_region((__sparse_force void __sparse_cache *)entry->sram_ptr, size); |
There was a problem hiding this comment.
There was a problem hiding this comment.
oh, yes, this looks correct and an oversight in the original implementation - thanks @serhiy-katsyuba-intel ! This should cover cases like multiple SRC instances on different cores using the same configuration, so they'll re-use the SRAM copy of the parameters (@singalsu will correct me if I'm mixing things up). So, @kv2019i, yes, this looks correct, and no, I don't think it's a part of a systemic oversight - this is a very specific fast-get aspect - sharing copies of data between instances. And fast-get context (struct sof_fast_get_entry instances) should be kept synchronised between cores too, but those are already allocated as uncached.
|
https://sof-ci.01.org/sofpr/PR10484/build18525/build/index.html should be harmless, but as it blocks Linux test execution, let me try to rekick CI once more. |
|
SOFCI TEST |
|
Looks good now, proceeding with merge. |
5 participants
fast_get() for the same data can be called by multiple clients from the same or other cores. Since sram_ptr points to cached memory, writeback is needed after memcpy() to also copy data to uncached memory. When fast_get() is called a second time for the same data, invalidate() is used to bring shared data into another core's cache.
This fixes tests which use two SRC modules.